Conversation
manifests/config.pp
Outdated
| default => "${foreman::foreman_service_bind}:${foreman::foreman_service_port}", | ||
| } | ||
|
|
||
| if $foreman::use_foreman_service { |
There was a problem hiding this comment.
Is there a best practice as to why this section does not live in service.pp since they are tied to the service itself?
There was a problem hiding this comment.
I think initially when I wrote this I considered them config files. Now looking back (and with the experiences from Pulpcore as well) I think service.pp would be a better place. For example, there's no reason this needs to be done before the DB, apache etc.
manifests/config.pp
Outdated
| keycloak_app_name => $foreman::keycloak_app_name, | ||
| keycloak_realm => $foreman::keycloak_realm, | ||
| if $foreman::apache and $foreman::ipa_authentication { | ||
| unless fact('foreman_ipa.default_server') and fact('foreman_ipa.default_realm') { |
There was a problem hiding this comment.
Not for this change, but I would have us consider putting external auth into it's own class for slimming down the config class and making it more obvious what aspects are tied to external auth.
There was a problem hiding this comment.
Probably, there's more room for improvement and I think I scratched the surface here.
| @@ -0,0 +1,65 @@ | |||
| # @summary The apache configuration for Foreman | |||
| # @api private | |||
| class foreman::apache { | |||
There was a problem hiding this comment.
👍 I like this being a clear, dedicated class
|
I'll revisit this once we've decided on dropping mod_passenger. |
ipa-getkeytab can figure out the default server on its own[1]. There is no need to specify it and can even break things. For example, DNS can be used to detect servers. Then the fact is empty and it fails while the command would actually pass. The foreman_ipa fact is removed since it's a major version bump anyway and nothing else should use our foreman_ipa fact. [1] theforeman#880 (comment)
Prior to this, when the Apache config was modified a full database refresh was triggered. There's no need for that and this makes applying those changes faster.
ekohl
force-pushed
the
prevent-db-reload-on-apache
branch
from
63813ca to
8d9a6f1
Compare
|
Since we've dropped mod_passenger for a while, I've rebased this. It includes #935 |
|
Can now be rebased again |
3 participants
Prior to this, when the Apache config was modified a full database refresh was triggered. There's no need for that and this makes applying those changes faster.
I found this while making the Pulpcore changes and this held me up. For now it's untested but pushing this allows me to test it out. Until then it's a draft.